Skip to content

CASSGO-27 fix gocql-RetryPolicy-still-dont-use-query-idempotence #1809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 19, 2024
Merged

CASSGO-27 fix gocql-RetryPolicy-still-dont-use-query-idempotence #1809

merged 1 commit into from
Nov 19, 2024

Conversation

@OleksiienkoMykyta
Copy link
Contributor

@OleksiienkoMykyta OleksiienkoMykyta commented Sep 3, 2024

Closes #1803
RetryPolicy doesn't check the query's idempotency, but according to the specification queries with false idempotence shouldn't be retried.

@SiyaoIsHiding
Copy link

SiyaoIsHiding commented Nov 4, 2024

Hi, I think this won't fix the issue because of this line

cassandra-gocql-driver/query_executor.go

Lines 67 to 69 in 92b4056

if !qry.IsIdempotent() || sp.Attempts() == 0 {
return q.do(qry.Context(), qry, hostIter), nil
}

the function do is only called when it's marked as NOT idempotent.

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 5, 2024

@SiyaoIsHiding that line you linked is to avoid speculative executions if the query isn't idempotent which seems correct to me... It's not really related to retries. If it's idempotent then run will be called which will then call do

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 5, 2024 •
edited
Loading

@OleksiienkoMykyta can you add a test that reproduces the bug before this change to ensure we don't get a regression in the future? It should be possible to reproduce this by using a very small timeout

@joao-r-reis joao-r-reis changed the title issue-1803, fix gocql-RetryPolicy-still-dont-use-query-idempotence CASSGO-27 fix gocql-RetryPolicy-still-dont-use-query-idempotence Nov 5, 2024
@OleksiienkoMykyta
Copy link
Contributor Author

OleksiienkoMykyta commented Nov 7, 2024

@OleksiienkoMykyta can you add a test that reproduces the bug before this change to ensure we don't get a regression in the future? It should be possible to reproduce this by using a very small timeout.

Yes, sure, I'll add some tests to cover this behavior.

@OleksiienkoMykyta
Copy link
Contributor Author

OleksiienkoMykyta commented Nov 7, 2024

@joao-r-reis I have added some tests, it would be nice if you would check it.
Also, I tried to test it with a timeout, but it doesn't reconcile query execution, just a single try, so I used a custom RetryPolicy.

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 7, 2024

TestQueryMultinodeWithMetrics is panicking because it assumes the retry policy runs for all hosts but now it doesn't because of the idempotency change.

joao-r-reis
joao-r-reis reviewed Nov 7, 2024
View reviewed changes
Copy link
Contributor

@joao-r-reis joao-r-reis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good 👍 , still need to fix failing tests though.

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the gocql-RetryPolicy-still-dont-use-query-idempotence branch from 9ce97bb to 072a04e Compare November 8, 2024 11:38
@OleksiienkoMykyta
Copy link
Contributor Author

OleksiienkoMykyta commented Nov 8, 2024

Changes look good 👍 , still need to fix failing tests though.

I have fixed it. The issue was in test which should make retries but doesn't set idempotence to query explicitly, now everything works properly.

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 12, 2024

Please format your commit message following the rules stated in CONTRIBUTING.md and add this JIRA to the CHANGELOG.md in the Fixed section of Unreleased, thanks!

We need another committer +1 to merge this.

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the gocql-RetryPolicy-still-dont-use-query-idempotence branch from 072a04e to a262096 Compare November 13, 2024 12:40
jfleming-ic
jfleming-ic reviewed Nov 18, 2024
View reviewed changes
CHANGELOG.md Outdated

### Fixed

- CASSGO-27
Copy link
Contributor

@jfleming-ic jfleming-ic Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have a short summary of what was fixed here, so users don't need to lookup what this ticket is, convention seems to be
"message" (ticket)

So for this, something like this would suffice

"Retry policy now takes into account query idempotency (CASSGO-27)"

Copy link
Contributor Author

@OleksiienkoMykyta OleksiienkoMykyta Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I have fixed it according to your suggestions

@Jacko161
Copy link

Jacko161 commented Nov 18, 2024

Sorry was on the wrong github account on my last comment - happy to tick this, but would be good to update the changelog message if possible.

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 19, 2024

And also change the first line of the commit message and add Jackson as a reviewer.

The first line of the commit message could be as an example: Change RetryPolicy so it checks query idempotence

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the gocql-RetryPolicy-still-dont-use-query-idempotence branch from a262096 to be5b5e2 Compare November 19, 2024 14:51
@OleksiienkoMykyta
Change RetryPolicy so it checks query idempotence
10750fe 
RetryPolicy doesn't check the query's idempotency,
but according to the specification queries with false idempotence shouldn't be retried.

patch by Mykyta Oleksiienko; reviewed by João Reis and Jackson Fleming for CASSGO-27
@OleksiienkoMykyta OleksiienkoMykyta force-pushed the gocql-RetryPolicy-still-dont-use-query-idempotence branch from be5b5e2 to 10750fe Compare November 19, 2024 14:54
@joao-r-reis joao-r-reis merged commit 109a892 into apache:trunk Nov 19, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jacko161 Jacko161 Jacko161 approved these changes

@joao-r-reis joao-r-reis joao-r-reis approved these changes

@jfleming-ic jfleming-ic Awaiting requested review from jfleming-ic

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CASSGO-27 gocql RetryPolicies still don't use query idempotence

5 participants

@OleksiienkoMykyta @SiyaoIsHiding @joao-r-reis @Jacko161 @jfleming-ic